Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @srfrog)

edgraph/server.go, line 400 at r2 (raw file):

		return nil, err
	}
	if req.StartTs == 0 {

It seems like this condition is impossible, since the mutation will be given a non-zero timestamp in the doMutate function. Maybe change it to an assert?

edgraph/server.go, line 470 at r2 (raw file):

	// Parse query and process
	var queryVars map[string][]string

This might be a controversial comment, and I know it already happens in some places of our code base, as reflected by the gocyclo score https://goreportcard.com/report/github.com/dgraph-io/dgraph#gocyclo .
I'm never a fan of super long functions, since it just makes it much harder for someone trying to understand the code.
Hence I would very much this newly added logic to be extracted out to a separate function.

gql/parser_mutation_test.go, line 153 at r2 (raw file):

		query {
			me(func: eq(email, "[email protected]")) {
				v as uid

what would happen if the variable ends up representing more than one uids, e.g.
me(func: has(email)) {
v as uid
}

also do we support have more than one variable bindings?
query{
lucas(func: eq(email, "[email protected]")) {
l as uid
}

gus(func: eq(email, "[email protected]")) {
g as uid
}
}

mutation {
set {
uid(l) uid(g)
}
}

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @gitlw)

edgraph/server.go, line 400 at r2 (raw file):

Sounds good

edgraph/server.go, line 470 at r2 (raw file):

I understand, but that's turning into a code cleanup which is not part of this feature. I'll try to move out as much logic as I can.

gql/parser_mutation_test.go, line 153 at r2 (raw file):

what would happen if the variable ends up representing more than one uids

the uids get collected in the queryVars slice, see GetUids(). laster we might have logic to run multiple mutations for each uid, but that hasn't been discussed yet.

also do we support have more than one variable bindings

yes, multiple vars are supported.

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @gitlw)

edgraph/server.go, line 400 at r2 (raw file):

Done.

edgraph/server.go, line 470 at r2 (raw file):

Done.

gql/parser_mutation_test.go, line 153 at r2 (raw file):

Done.

Reviewable status: 0 of 10 files reviewed, 8 unresolved discussions (waiting on @gitlw and @srfrog)

chunker/rdf/parse.go, line 188 at r3 (raw file):

		rnq.ObjectValue = &api.Value{Val: &api.Value_DefaultVal{DefaultVal: oval}}
	}
	if (len(rnq.Subject) == 0 && rnq.VarName == nil) || len(rnq.Predicate) == 0 {

It seems we should not add the logical AND of checking var name here,
because even when the VarName is not nil, the Subject should still not be empty, right?

chunker/rdf/parse.go, line 194 at r3 (raw file):

		return rnq, x.Errorf("No Object in NQuad. Input: [%s]", line)
	}
	if (!sane(rnq.Subject) && rnq.VarName == nil) || !sane(rnq.Predicate) ||

Same argument as above, even when the VarName is not nil, the Subject should still be sane.

edgraph/server.go, line 427 at r3 (raw file):

	if len(queryVars) == 0 {
		// TODO: remove this error when mutation conditional expressions are added.
		return nil, x.Errorf("No variables defined in conditional query: %q", req.Query)

As we discussed, there should not be an error when the conditional query does not match any uid.

gql/parser.go, line 589 at r3 (raw file):

	if len(defines) > len(needs) {
		// return x.Errorf("Some variables are defined but not used\nDefined:%v\nUsed:%v\n",

Since a regular query parsing also uses this code block, this change would affect those as well.
For now, maybe put a comment explaining that the defined variables for a conditional query are not used in the query block, but the mutation block below.

gql/parser_mutation.go, line 67 at r3 (raw file):

		if item.Typ == itemRightCurl {
			// mutations must be enclosed in a single block.
			if !inTxn && it.Next() && it.Item().Typ != lex.ItemEOF {

Adding this extra condition for reporting error would make the parser accept input like
txn {
query{
...}
mutation {
...}
some unexpected input
}
and that's not good.

Reviewable status: 0 of 10 files reviewed, 13 unresolved discussions (waiting on @gitlw and @srfrog)

gql/parser_mutation.go, line 86 at r3 (raw file):

func parseMutationCondQuery(it *lex.ItemIterator) (string, error) {
	var query string
	var parse bool

maybe rename this var to "hasQuery"?

gql/parser_mutation.go, line 92 at r3 (raw file):

		case itemLeftCurl:
			continue
		case itemMutationContent:

I think this should be renamed to itemCondQueryOrMutationContent

gql/parser_mutation_test.go, line 291 at r3 (raw file):

	_, err := ParseMutation(m)
	require.Error(t, err)
	require.Contains(t, err.Error(), `Invalid character '}' inside query text`)

The error msg "Invalid character '}' inside query text" does not seem to be very helpful in finding out where the parsing error happens. Can you please double check the whole error msg and make sure the line and column numbers work correctly?

gql/state.go, line 437 at r3 (raw file):

		switch word {
		case "query":
			l.Emit(itemMutationTxnOp)

Do you feel it's better to have separate item values to represent the query and mutation inside a txn?

gql/state.go, line 475 at r3 (raw file):

}

func lexTextQuery(l *lex.Lexer) lex.StateFn {

Maybe rename to lexCondQuery?

Reviewable status: 0 of 10 files reviewed, 13 unresolved discussions (waiting on @gitlw and @srfrog)

chunker/rdf/parse.go, line 188 at r3 (raw file):

There won't be a subject because there's no <> parsed, it uses a keyword, uid.

See: https://github.com/dgraph-io/dgraph/blob/srfrog/issue-3059_new_txn_operation/chunker/rdf/state.go#L79

But I think we need to make rnq.SubjectVarName and rnq.ObjectVarName instead. So if rnq.Subject and rnq.SubjectVarName are empty, then that's an error. Same with object.

chunker/rdf/parse.go, line 194 at r3 (raw file):

Like above, subject content never gets parsed.

gql/parser_mutation.go, line 67 at r3 (raw file):

That is true for txn but not for regular mutations. The side effect is that anything after the mutation's last } is ignored as comment because parsing has stopped.

I will work on this some more when other things are solid. This means changing the lexer which could mean updating to all the parsing.

gql/parser_mutation.go, line 86 at r3 (raw file):

all the parsing functions have a signature parse... like parseMutationOp below this one, I'm just following that.

gql/parser_mutation.go, line 92 at r3 (raw file):

that sounds like it could break some law of thermodynamics.
how about itemMutationOpContent ? since query and mutation are operators.

gql/state.go, line 437 at r3 (raw file):

yes, they are orthogonal values. there is really nothing preventing us from adding a new schema{} block in there, for example.

gql/state.go, line 475 at r3 (raw file):

it can be lexTextCondQuery we need text part to indicate what we emit.

Reviewable status: 0 of 10 files reviewed, 13 unresolved discussions (waiting on @gitlw and @srfrog)

edgraph/server.go, line 427 at r3 (raw file):

Done.

gql/parser.go, line 589 at r3 (raw file):

Done.

Reviewable status: 0 of 10 files reviewed, 13 unresolved discussions (waiting on @gitlw)

gql/parser_mutation_test.go, line 291 at r3 (raw file):

Changed error and now it reads:

...at line 3 column 8: Unbalanced '}' found inside query text" inside of txn block."

Correctly indicating the beginning of the query block and that the brace is unbalanced.

Reviewed 1 of 10 files at r1, 2 of 5 files at r2, 8 of 8 files at r4.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @gitlw and @srfrog)

chunker/rdf/parse.go, line 84 at r4 (raw file):

			switch item.Typ {
			case itemSubjectVarName:
				rnq.SubjectVar = item.Val

Try to get rid of NQuad changes.

edgraph/server.go, line 389 at r4 (raw file):

// doCondQuery processes a conditional query within the same transaction of a mutation.
// Returns a map with the vars created from the query, otherwise nil and an error.
func doCondQuery(ctx context.Context, l *query.Latency, req *api.Request,

Can share a lot of logic with the Query func. So, refactor that out in another func and use it in both Query and txn mutation.

gql/parser_mutation.go, line 48 at r4 (raw file):

		var err error
		// Get the query text: txn{ query { ... }}
		mu.CondQuery, err = parseMutationCondQuery(it)

mu.Query, err = parseMutationQuery(it)